Java: Improve the ExecTainted query #4287

joefarebrother · 2020-09-17T16:01:25Z

This PR adds most of the results that #4051 does, but with fewer FPs resultng from user input flowing to arguments of non-shell commands.

aschackmull · 2020-09-18T13:28:45Z

I've started a query to find the newly filtered results: https://lgtm.com/query/695684662946170257/

java/ql/src/semmle/code/java/security/CommandArguments.qll

aschackmull

Some inline comments, otherwise looks good. The LGTM query run should give us a sense of the impact of these changes.

aschackmull · 2020-09-18T14:20:06Z

Hmm, just tried this on JDK11. Looks like there's a performance problem in ImmutableFirstArrayExpr. One indication is the size of this join_rhs:

[2020-09-18 16:17:32] (162s) Tuple counts for CommandArguments::ImmutableFirstArrayExpr#class#f#join_rhs#4:
                      7418433  ~6%     {2} r1 = SCAN Expr::Expr::getType_dispred#ff AS I OUTPUT I.<1>, I.<0>
                      193714   ~0%     {3} r2 = JOIN r1 WITH CommandArguments::ArrayOfStringType#class#f AS R ON FIRST 1 OUTPUT "java.util", "Arrays", r1.<1>
                      193714   ~0%     {2} r3 = JOIN r2 WITH Type::RefType::hasQualifiedName_dispred#bff_120#join_rhs AS R ON FIRST 2 OUTPUT R.<2>, r2.<2>
                      47266216 ~1%     {3} r4 = JOIN r3 WITH Member::Member::getDeclaringType_dispred#ff_10#join_rhs AS R ON FIRST 1 OUTPUT R.<1>, "copyOf", r3.<1>
                      1937140  ~5%     {2} r5 = JOIN r4 WITH Element::Element::hasName_dispred#ff AS R ON FIRST 2 OUTPUT r4.<0>, r4.<2>
                      93951290 ~0%     {3} r6 = JOIN r5 WITH Expr::MethodAccess::getMethod_dispred#ff_10#join_rhs AS R ON FIRST 1 OUTPUT R.<1>, 0, r5.<1>
                      93951290 ~4%     {2} r7 = JOIN r6 WITH Expr::MethodAccess::getArgument_dispred#fff AS R ON FIRST 2 OUTPUT R.<2>, r6.<2>
                                       return r7
[2020-09-18 16:17:33] (163s) Stopped the world to evict 257874211 ARRAYS at sequence stamp o+22420191
[2020-09-18 16:17:33] (163s) Starting the world again: 259025536 bytes forgotten up to a UNIMPORTANT item with sequence stamp o+7688682
[2020-09-18 16:17:34] (164s) Stopped the world to evict 257874211 ARRAYS at sequence stamp o+22420818
[2020-09-18 16:17:34] (164s) Starting the world again: 259025536 bytes forgotten up to a UNIMPORTANT item with sequence stamp o+7897450
[2020-09-18 16:17:35] (165s) Registering CommandArguments::ImmutableFirstArrayExpr#class#f#join_rhs#4/2@a8d1a3 + [] with content 27c5130ebcg5e412i2nko3rmiac80
[2020-09-18 16:17:35] (165s)  >>> Wrote relation CommandArguments::ImmutableFirstArrayExpr#class#f#join_rhs#4/2@a8d1a3 with 93951290 rows and 2 columns.

aschackmull · 2020-09-18T14:21:01Z

And the first delta (i.e. iteration 2) of ImmutableFirstArrayExpr doesn't appear to terminate within any reasonable time.

joefarebrother · 2020-09-18T16:18:02Z

I think there's a bug in your LGTM query - since DataFlow::PathNode includes the configuration, the line not fix.hasFlowPath(source, sink) is always going to be true, so it's just going to give exactly the results of the original query.

aschackmull · 2020-09-22T11:13:59Z

Thanks, yes, here's the fixed version: https://lgtm.com/query/7616579728577345448/
Results are looking good.

joefarebrother added 3 commits September 17, 2020 15:39

Java: Improve the ExecTainted query

6bfc0af

Java: Make the equivalent changes to ExecTaintedLocal

b6cf1cc

Java: Add tests for ExecTainted

fcfc836

joefarebrother requested a review from a team September 17, 2020 16:01

github-actions bot added the Java label Sep 17, 2020

joefarebrother added 3 commits September 17, 2020 17:13

Java: Fix formatting

810baad

Java: Fix QLDoc

69fd579

Java: Fix formatting

9c643ec